Skip to content

GH-49991: [C++][FlightRPC] Fix unity build ordering issue#49990

Merged
kou merged 2 commits into
apache:mainfrom
spotaws:fix-unity-build
May 20, 2026
Merged

GH-49991: [C++][FlightRPC] Fix unity build ordering issue#49990
kou merged 2 commits into
apache:mainfrom
spotaws:fix-unity-build

Conversation

@spotaws
Copy link
Copy Markdown
Contributor

@spotaws spotaws commented May 19, 2026 •
edited by github-actions Bot
Loading

Rationale for this change

Original error message:

/home/spot/git/sandbox/libarrow/libarrow-24.0.0-build/apache-arrow-24.0.0/cpp/src/arrow/flight/transport/grpc/grpc_client.cc: In member function ‘virtual arrow::Status
  arrow::flight::transport::grpc::{anonymous}::GrpcClientImpl::Init(const arrow::flight::FlightClientOptions&, const arrow::flight::Location&, const arrow::util::Uri&)’:
  /home/spot/git/sandbox/libarrow/libarrow-24.0.0-build/apache-arrow-24.0.0/cpp/src/arrow/flight/transport/grpc/grpc_client.cc:738:25: error: ‘FromAbslStatus’ was not declared in this scope; did you mean
  ‘FromGrpcStatus’?
    738 |           RETURN_NOT_OK(FromAbslStatus(certificate_provider->UpdateRoot(kDummyRootCert)));
        |                         ^~~~~~~~~~~~~~
  /home/spot/git/sandbox/libarrow/libarrow-24.0.0-build/apache-arrow-24.0.0/cpp/src/arrow/status.h:58:45: note: in definition of macro ‘ARROW_RETURN_NOT_OK’
     58 |     ::arrow::Status __s = ::arrow::ToStatus(status);           \
        |                                             ^~~~~~
  /home/spot/git/sandbox/libarrow/libarrow-24.0.0-build/apache-arrow-24.0.0/cpp/src/arrow/flight/transport/grpc/grpc_client.cc:738:11: note: in expansion of macro ‘RETURN_NOT_OK’
    738 |           RETURN_NOT_OK(FromAbslStatus(certificate_provider->UpdateRoot(kDummyRootCert)));
        |           ^~~~~~~~~~~~~
  In file included from /home/spot/git/sandbox/libarrow/libarrow-24.0.0-build/apache-arrow-24.0.0/cpp/redhat-linux-build/src/arrow/flight/CMakeFiles/arrow_flight_objlib.dir/Unity/unity_1_cxx.cxx:25:
  /home/spot/git/sandbox/libarrow/libarrow-24.0.0-build/apache-arrow-24.0.0/cpp/src/arrow/flight/transport/grpc/util_internal.cc: At global scope:
  /home/spot/git/sandbox/libarrow/libarrow-24.0.0-build/apache-arrow-24.0.0/cpp/src/arrow/flight/transport/grpc/util_internal.cc:335:8: warning: no previous declaration for ‘arrow::Status
  arrow::flight::transport::grpc::FromAbslStatus(const absl::lts_20260107::Status&)’ [-Wmissing-declarations]
    335 | Status FromAbslStatus(const ::absl::Status& absl_status) {
        |        ^~~~~~~~~~~~~~

In testing builds against latest grpc and protobuf, I found a Unity build (CMake UNITY_BUILD) ordering issue. The file server_tracing_middleware.cc is the first file in the Unity translation unit and includes util_internal.h without first including <grpcpp/grpcpp.h>. Since util_internal.h uses #pragma once, it's only processed once — at that point GRPC_CPP_VERSION_MAJOR is undefined, so the #if GRPC_CPP_VERSION_CHECK(1, 80, 0) guard evaluates to false and the FromAbslStatus declaration is skipped. When grpc_client.cc later tries to call FromAbslStatus, the declaration was never emitted.

What changes are included in this PR?

I added #include <grpcpp/version_info.h> at the top so the version macros are always available, moved the GRPC_CPP_VERSION_CHECK macro definition before the namespace block (with a safety check for the macros being defined), and added #include <absl/status/status.h> conditionally when gRPC >= 1.80.0

Are these changes tested?

Yes, I ran the full test suite with this change applied (and the build now succeeds).

100% tests passed, 0 tests failed out of 97

Label Time Summary:
arrow-compute-tests = 7.32 secproc (14 tests)
arrow-tests = 24.99 secproc (40 tests)
arrow_acero = 26.32 secproc (14 tests)
arrow_dataset = 27.48 secproc (14 tests)
arrow_flight = 2.00 secproc (2 tests)
filesystem = 0.86 secproc (2 tests)
parquet-tests = 11.88 secproc (13 tests)
unittest = 99.98 secproc (97 tests)

Total Test time (real) = 15.10 sec

Are there any user-facing changes?

No.

@spotaws
fix Unity build ordering issue
a0ff085 
Signed-off-by: Tom spot Callaway <spotaws@amazon.com>
@spotaws spotaws requested a review from lidavidm as a code owner May 19, 2026 15:01
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 19, 2026

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

@spotaws spotaws changed the title fix Unity build ordering issue GH-49991: [C++] fix Unity build ordering issue May 19, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 19, 2026

⚠️ GitHub issue #49991 has been automatically assigned in GitHub to PR creator.

lidavidm
lidavidm approved these changes May 19, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tracking this down - there's just a formatter error it seems

@kou kou changed the title GH-49991: [C++] fix Unity build ordering issue GH-49991: [C++][FlightRPC] Fix Unity build ordering issue May 19, 2026
@kou kou changed the title GH-49991: [C++][FlightRPC] Fix Unity build ordering issue GH-49991: [C++][FlightRPC] Fix unity build ordering issue May 19, 2026
kou
kou reviewed May 19, 2026
View reviewed changes
Comment thread cpp/src/arrow/flight/transport/grpc/util_internal.h Outdated
Comment on lines +27 to +28
#if defined(GRPC_CPP_VERSION_MAJOR) && defined(GRPC_CPP_VERSION_MINOR) && \
defined(GRPC_CPP_VERSION_PATCH)
Copy link
Copy Markdown
Member

@kou kou May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need these checks?

It seems that grpcpp/version_info.h always provides them.

Copy link
Copy Markdown
Contributor Author

@spotaws spotaws May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was being cautious and trying to future proof, but maybe it's overkill.

Copy link
Copy Markdown
Member

@kou kou May 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If they are not needed, could you remove them to simplify our implementation?

@kou kou requested a review from Copilot May 20, 2026 01:09
Copilot AI reviewed May 20, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a CMake UNITY_BUILD ordering problem in the Flight gRPC transport where util_internal.h could be processed before gRPC version macros were available, causing FromAbslStatus to not be declared (and later calls to fail to compile).

Changes:

  • Include <grpcpp/version_info.h> in util_internal.h so GRPC_CPP_VERSION_* macros are available regardless of include order.
  • Move/adjust GRPC_CPP_VERSION_CHECK macro definition so it is usable before the namespace block (with a fallback when version macros aren’t defined).
  • Conditionally include <absl/status/status.h> when building against gRPC >= 1.80.0 so ::absl::Status is known for the conditional declaration.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

kou
kou reviewed May 20, 2026
View reviewed changes
Comment thread cpp/src/arrow/flight/transport/grpc/util_internal.h Outdated
kou
kou approved these changes May 20, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@kou kou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@kou kou merged commit eab3d11 into apache:main May 20, 2026
51 of 55 checks passed
@kou kou removed the awaiting review Awaiting review label May 20, 2026
@kou kou mentioned this pull request May 20, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@kou kou kou approved these changes

@lidavidm lidavidm lidavidm approved these changes

Assignees

No one assigned

Labels

Component: C++ Component: FlightRPC

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@spotaws @kou @lidavidm